feat(zfs-localpv): add option for choosing between refquota and quota #542

cinapm · 2024-06-13T10:31:23Z

Why is this PR required? What issue does it fix?:
resolve this issue: #423

What this PR does?:
this PR adds an option for choosing between refquota or quota and uses this value as zfs command options

Checklist:

Fixes #
PR Title follows the convention of <type>(<scope>): <subject>
Has the change log section been updated?
Commit has unit tests
Commit has integration tests
(Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
(Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

codecov-commenter · 2024-06-14T13:12:40Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.37%. Comparing base (e3d55d7) to head (ae2b7c2).
Report is 2 commits behind head on develop.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #542   +/-   ##
========================================
  Coverage    96.37%   96.37%           
========================================
  Files            1        1           
  Lines          496      496           
========================================
  Hits           478      478           
  Misses          14       14           
  Partials         4        4

Flag	Coverage Δ
bddtests	`96.37% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/main.go

pkg/config/config.go

cmd/main.go

Abhinandan-Purkait · 2024-06-20T11:44:59Z

Thanks @cinapm. Looks good to me. I will let other folks review this. We will target this for this upcoming release.

dsharma-dc · 2024-06-20T16:52:38Z

Have we tested this change by trying to apply a quota, and then a refquota underneath that dataset?
Add a simple test as well perhaps.

niladrih

This configuration option should be toggle-able through a storageclass.

Abhinandan-Purkait · 2024-07-09T05:44:45Z

@cinapm Can you please let us know your plans on this PR? I would request you to consider the suggestion made by @niladrih here.

cinapm · 2024-07-11T16:38:09Z

This configuration option should be toggle-able through a storageclass.

i have added option to add quotatype parameter on storageclass, the default value is quota

cinapm · 2024-07-11T16:39:36Z

@cinapm Can you please let us know your plans on this PR? I would request you to consider the suggestion made by @niladrih here.

instead of using config for local-pv, I have changed the controller to accept and use quotatype parameter on storageclass

Abhinandan-Purkait · 2024-07-15T14:18:18Z

@cinapm Thanks, we will review this. The team needs to decide on this because this changes the CRDs and changing anything on the CRDs would need a CRD version bump which now raises the question of upgrades and migration. cc @niladrih @avishnu @tiagolobocastro

abuisine · 2024-07-15T16:28:37Z

I would strongly suggest to verify the impact on the thick provisioning side of things :
with a thinprovision: "no" the property reservation is set, it would probably be mandatory to switch to refreservation at the same time that refquota is used.

Let me explain the potential problem with the typical usage of snapshots :

snapshots are considered in quota but are not when using refquota
snapshots are considered in reservation but are not when using refreservation

You could end up with a non functionnal thick provisioning while mixing refquota with reservation

Let me take an exemple with a workload that requires 10GB of free space, with thick provisioning :

a PersitentVolume of 10GB
refquota and reservation being set (through thinprovision: "no")
with refquota, you can create an unlimited amount of GB of snapshots
with reservation, snapshots are considered as consuming space
this would mean that with 3GB of space being used and 9GB of snapshots, there would be 7GB of free space from a zfs FS standpoint, and that reservation would be exceeded by 2GB
so 2GB would be thin provisioned whereas the configuration is set to thick provisioning

Using refreservation while setting refquota and thinprovision: "no" would solve the issue.

abuisine · 2024-07-15T16:36:38Z

Also, I would like to confirm the huge value of this PR : without refquota and refreservation the usage of snapshots becomes almost impossible as they are deduced from free space.
From an operational standpoint, it basically means that backups can impact production.
We never used snapshots with this CSI for this reason.

This PR (with refreservation) would enable safe usage of snapshots in production.

cinapm · 2024-07-15T22:10:49Z

I would strongly suggest to verify the impact on the thick provisioning side of things : with a thinprovision: "no" the property reservation is set, it would probably be mandatory to switch to refreservation at the same time that refquota is used.

Let me explain the potential problem with the typical usage of snapshots :

snapshots are considered in quota but are not when using refquota

snapshots are considered in reservation but are not when using refreservation

You could end up with a non functionnal thick provisioning while mixing refquota with reservation

Let me take an exemple with a workload that requires 10GB of free space, with thick provisioning :

a PersitentVolume of 10GB

refquota and reservation being set (through thinprovision: "no")

with refquota, you can create an unlimited amount of GB of snapshots

with reservation, snapshots are considered as consuming space

this would mean that with 3GB of space being used and 9GB of snapshots, there would be 7GB of free space from a zfs FS standpoint, and that reservation would be exceeded by 2GB

so 2GB would be thin provisioned whereas the configuration is set to thick provisioning

Using refreservation while setting refquota and thinprovision: "no" would solve the issue.

Your point is very valid, and based on the points you mentioned, I have made the changes. now when there is option thinprovision: "no" , the reservation will be set for quota and refreservation will be set for refquota

pkg/driver/controller.go

pkg/zfs/zfs_util.go

Abhinandan-Purkait

@cinapm A couple of issues here I see you have changed the v1/zfsvolume.go file and also the crd and operator yamls. The CRDs are supposed to be generated from the v1/zfsvolume.go file and are not supposed to be changed manually. It would be good if you undo the yaml changes and do it via the script. Also, since you have changed the v1/zfsvolume.go we also need to do the kubegen again.

make bootstrap
make kubegen
make manifests

One more thing, I see you changed the zfs-restore and zfs-snapshot crds as well, but I don't see any equivalent changes on their corresponding go files. So if you want to change them as well, you need to change the go files first and run the above scripts.

cinapm · 2024-07-18T20:44:17Z

make manifests
@Abhinandan-Purkait

i didn't change them manually, first of all I changed v1/zfsvolume.go and after that I have used following commands:
make bootstrap make manifests

the make manifests command has changed zfs-restore and zfs-snapshot crds automatically based on changes in v1/zfsvolume.go

Abhinandan-Purkait · 2024-07-19T05:44:52Z

make manifests
@Abhinandan-Purkait

i didn't change them manually, first of all I changed v1/zfsvolume.go and after that I have used following commands: make bootstrap make manifests

the make manifests command has changed zfs-restore and zfs-snapshot crds automatically based on changes in v1/zfsvolume.go

Okay, I see the embedded volSpec in the restores and snapshot structures. Okay. It would help if you rebased your branch, there were some changes merged on the deepcopy methods. Also please run make kubegen although it won't change anything in your case, but it's worth checking.

You also need to copy the crd changes into the helm chart crd templates, as that's how crds get installed. Check https://github.com/openebs/zfs-localpv/tree/develop/deploy/helm/charts/charts/crds/templates

The other changes look good to me. It might be worth testing upgrade once before we take this in. Example, installing the previous version of zfs and upgrading it to this version and see the crd changes. Thanks

pkg/zfs/zfs_util.go

cinapm · 2024-07-19T15:11:44Z

make manifests
@Abhinandan-Purkait

i didn't change them manually, first of all I changed v1/zfsvolume.go and after that I have used following commands: make bootstrap make manifests
the make manifests command has changed zfs-restore and zfs-snapshot crds automatically based on changes in v1/zfsvolume.go

Okay, I see the embedded volSpec in the restores and snapshot structures. Okay. It would help if you rebased your branch, there were some changes merged on the deepcopy methods. Also please run make kubegen although it won't change anything in your case, but it's worth checking.

You also need to copy the crd changes into the helm chart crd templates, as that's how crds get installed. Check https://github.com/openebs/zfs-localpv/tree/develop/deploy/helm/charts/charts/crds/templates

The other changes look good to me. It might be worth testing upgrade once before we take this in. Example, installing the previous version of zfs and upgrading it to this version and see the crd changes. Thanks

@Abhinandan-Purkait

as you said, first of all, I have rebased my branch and after that I added crds changes into the corresponding helm manifests

Abhinandan-Purkait

LGTM. Test the upgrades before this can be taken in.

sinhaashish · 2024-07-31T06:03:25Z

@cinapm It will be good if you add the test case to check the refquota and refreservation
Something on this lines is alredy present here

zfs-localpv/tests/utils.go

Line 131 in 2f93897

func generateThinProvisionParams(property map[string]string) {

You can find the details about this is this issue #560
The conclusion is here #560 (comment)

Abhinandan-Purkait · 2024-08-01T06:33:51Z

@cinapm Please rebase and run make manifests a lot of changes went in on the testing and ci side of things. Thanks

Abhinandan-Purkait · 2024-08-22T06:44:42Z

@cinapm I see there isn't any activity here for a while, is there any blocker that you are facing that you need help with?

Signed-off-by: cina_pm <[email protected]>

…rovisioning Signed-off-by: cina_pm <[email protected]>

Signed-off-by: cina_pm <[email protected]>

…g switch case Signed-off-by: cina_pm <[email protected]>

… manifests Signed-off-by: cina_pm <[email protected]>

Signed-off-by: cina_pm <[email protected]>

cinapm · 2024-09-06T16:15:07Z

@cinapm I see there isn't any activity here for a while, is there any blocker that you are facing that you need help with?

I have rebased the develop branch and also run make manifests

cinapm · 2024-09-06T16:15:46Z

@cinapm Please rebase and run make manifests a lot of changes went in on the testing and ci side of things. Thanks

@Abhinandan-Purkait
I have resolved them

Abhinandan-Purkait

LGTM

Abhinandan-Purkait · 2024-09-09T07:20:16Z

@niladrih @tiagolobocastro @sinhaashish PTAL

sinhaashish · 2024-09-10T03:52:35Z

I am approving it , but better add the test here here

sinhaashish · 2024-09-17T06:31:21Z

@cinapm Just making sure there’s nothing pending on your side. If everything looks good, we can go ahead and merge it

cinapm · 2024-09-18T06:20:13Z

@cinapm Just making sure there’s nothing pending on your side. If everything looks good, we can go ahead and merge it

@sinhaashish
there is nothing on my side, everything is ok

Abhinandan-Purkait · 2024-09-21T16:54:41Z

@cinapm Thanks. Merging the PR.

Abhinandan-Purkait reviewed Jun 18, 2024

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

pkg/config/config.go Outdated Show resolved Hide resolved

cmd/main.go Outdated Show resolved Hide resolved

Abhinandan-Purkait requested review from tiagolobocastro, sinhaashish and niladrih June 18, 2024 05:50

Abhinandan-Purkait approved these changes Jun 20, 2024

View reviewed changes

niladrih reviewed Jun 21, 2024

View reviewed changes

niladrih mentioned this pull request Jun 21, 2024

Option to choose refquota zfs field to apply quotas #423

Open

cinapm force-pushed the develop branch from f8be4da to ab2b941 Compare June 23, 2024 07:32

cinapm requested a review from niladrih June 23, 2024 07:37

cinapm force-pushed the develop branch from ab2b941 to 8a40fbd Compare June 23, 2024 08:48

cinapm force-pushed the develop branch from 8a40fbd to a87da86 Compare July 11, 2024 16:36

Abhinandan-Purkait mentioned this pull request Jul 16, 2024

No clear default value for thin provisioning of filesystems #560

Closed

sinhaashish reviewed Jul 16, 2024

View reviewed changes

pkg/driver/controller.go Outdated Show resolved Hide resolved

sinhaashish reviewed Jul 16, 2024

View reviewed changes

pkg/driver/controller.go Outdated Show resolved Hide resolved

sinhaashish reviewed Jul 16, 2024

View reviewed changes

pkg/zfs/zfs_util.go Outdated Show resolved Hide resolved

Abhinandan-Purkait requested changes Jul 17, 2024

View reviewed changes

cinapm requested a review from Abhinandan-Purkait July 18, 2024 20:45

Abhinandan-Purkait reviewed Jul 19, 2024

View reviewed changes

pkg/zfs/zfs_util.go Outdated Show resolved Hide resolved

cinapm requested a review from sinhaashish July 19, 2024 14:29

cinapm force-pushed the develop branch from 5321c2f to 68d49ed Compare July 19, 2024 14:56

cinapm requested a review from Abhinandan-Purkait July 19, 2024 15:21

Abhinandan-Purkait approved these changes Jul 22, 2024

View reviewed changes

sinhaashish approved these changes Jul 23, 2024

View reviewed changes

cinapm added 7 commits September 6, 2024 19:38

feat(zfs-localpv): rebase from upstream develop branch

03fa5f0

Signed-off-by: cina_pm <[email protected]>

feat(zfs-localpv): add refreservation for refquota when using Thick p…

85a223f

…rovisioning Signed-off-by: cina_pm <[email protected]>

fix(zfs-localpv): fix typo qoutatype -> quotatype

d7fe88c

Signed-off-by: cina_pm <[email protected]>

fix(zfs-localpv): replace quota case with default in thin provisionin…

1d8363a

…g switch case Signed-off-by: cina_pm <[email protected]>

feat(zfs-localpv): add deploy/crds into corresponding deploy/helm crd…

f31712c

… manifests Signed-off-by: cina_pm <[email protected]>

feat(zfs-localpv): add reservationProperty function in zfs_util

f01fd2f

Signed-off-by: cina_pm <[email protected]>

feat(zfs-localpv): make manifests

ae2b7c2

Signed-off-by: cina_pm <[email protected]>

cinapm force-pushed the develop branch from ae7b567 to ae2b7c2 Compare September 6, 2024 16:13

Abhinandan-Purkait approved these changes Sep 9, 2024

View reviewed changes

Abhinandan-Purkait requested a review from sinhaashish September 9, 2024 07:19

sinhaashish approved these changes Sep 10, 2024

View reviewed changes

Abhinandan-Purkait merged commit f185333 into openebs:develop Sep 21, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(zfs-localpv): add option for choosing between refquota and quota #542

feat(zfs-localpv): add option for choosing between refquota and quota #542

cinapm commented Jun 13, 2024 •

edited

Loading

codecov-commenter commented Jun 14, 2024 •

edited

Loading

Abhinandan-Purkait commented Jun 20, 2024

dsharma-dc commented Jun 20, 2024

niladrih left a comment

Abhinandan-Purkait commented Jul 9, 2024

cinapm commented Jul 11, 2024

cinapm commented Jul 11, 2024

Abhinandan-Purkait commented Jul 15, 2024

abuisine commented Jul 15, 2024

abuisine commented Jul 15, 2024 •

edited

Loading

cinapm commented Jul 15, 2024

Abhinandan-Purkait left a comment

cinapm commented Jul 18, 2024 •

edited

Loading

Abhinandan-Purkait commented Jul 19, 2024 •

edited

Loading

cinapm commented Jul 19, 2024

Abhinandan-Purkait left a comment

sinhaashish commented Jul 31, 2024

Abhinandan-Purkait commented Aug 1, 2024

Abhinandan-Purkait commented Aug 22, 2024

cinapm commented Sep 6, 2024

cinapm commented Sep 6, 2024

Abhinandan-Purkait left a comment

Abhinandan-Purkait commented Sep 9, 2024

sinhaashish commented Sep 10, 2024

sinhaashish commented Sep 17, 2024

cinapm commented Sep 18, 2024

Abhinandan-Purkait commented Sep 21, 2024

feat(zfs-localpv): add option for choosing between refquota and quota #542

feat(zfs-localpv): add option for choosing between refquota and quota #542

Conversation

cinapm commented Jun 13, 2024 • edited Loading

codecov-commenter commented Jun 14, 2024 • edited Loading

Codecov Report

Abhinandan-Purkait commented Jun 20, 2024

dsharma-dc commented Jun 20, 2024

niladrih left a comment

Choose a reason for hiding this comment

Abhinandan-Purkait commented Jul 9, 2024

cinapm commented Jul 11, 2024

cinapm commented Jul 11, 2024

Abhinandan-Purkait commented Jul 15, 2024

abuisine commented Jul 15, 2024

abuisine commented Jul 15, 2024 • edited Loading

cinapm commented Jul 15, 2024

Abhinandan-Purkait left a comment

Choose a reason for hiding this comment

cinapm commented Jul 18, 2024 • edited Loading

Abhinandan-Purkait commented Jul 19, 2024 • edited Loading

cinapm commented Jul 19, 2024

Abhinandan-Purkait left a comment

Choose a reason for hiding this comment

sinhaashish commented Jul 31, 2024

Abhinandan-Purkait commented Aug 1, 2024

Abhinandan-Purkait commented Aug 22, 2024

cinapm commented Sep 6, 2024

cinapm commented Sep 6, 2024

Abhinandan-Purkait left a comment

Choose a reason for hiding this comment

Abhinandan-Purkait commented Sep 9, 2024

sinhaashish commented Sep 10, 2024

sinhaashish commented Sep 17, 2024

cinapm commented Sep 18, 2024

Abhinandan-Purkait commented Sep 21, 2024

cinapm commented Jun 13, 2024 •

edited

Loading

codecov-commenter commented Jun 14, 2024 •

edited

Loading

abuisine commented Jul 15, 2024 •

edited

Loading

cinapm commented Jul 18, 2024 •

edited

Loading

Abhinandan-Purkait commented Jul 19, 2024 •

edited

Loading